Skip to content

Introduce the extraProperties to support user-defined pluggable accessCheckers#42

Merged
duanmeng merged 5 commits intoapache:masterfrom
zuston:delegateAccessFix2
Jul 18, 2022
Merged

Introduce the extraProperties to support user-defined pluggable accessCheckers#42
duanmeng merged 5 commits intoapache:masterfrom
zuston:delegateAccessFix2

Conversation

@zuston
Copy link
Member

@zuston zuston commented Jul 8, 2022

What changes were proposed in this pull request?

Introduce the reservedData to extend more pluggable accessCheckers

Why are the changes needed?

In current codebase, the accessinfo only have acessid and tags. If we want to extend more AccessChecker in coordinator, the info is not enough.

To solve this, i think introducing the reservedData is necessary.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UTs

@roryqi
Copy link
Contributor

roryqi commented Jul 8, 2022

Could you give us some examples?

@zuston zuston mentioned this pull request Jul 8, 2022
Closed
3 tasks
@zuston
Copy link
Member Author

zuston commented Jul 12, 2022

Could u help check this PR? @duanmeng

duanmeng
duanmeng reviewed Jul 12, 2022
View reviewed changes
@zuston zuston force-pushed the delegateAccessFix2 branch from aa42109 to a028f75 Compare July 16, 2022 02:16
@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2022 •
edited
Loading

Codecov Report

Merging #42 (346c6d1) into master (f931cf0) will decrease coverage by 0.05%.
The diff coverage is 25.00%.

@@             Coverage Diff              @@
##             master      #42      +/-   ##
============================================
- Coverage     55.18%   55.12%   -0.06%     
+ Complexity     1112     1110       -2     
============================================
  Files           149      149              
  Lines          7964     7969       +5     
  Branches        760      761       +1     
============================================
- Hits           4395     4393       -2     
- Misses         3328     3333       +5     
- Partials        241      243       +2
Impacted Files Coverage Δ
...he/uniffle/coordinator/CoordinatorGrpcService.java 2.42% <0.00%> (-0.05%) ⬇️
...ava/org/apache/uniffle/coordinator/AccessInfo.java 81.81% <50.00%> (-18.19%) ⬇️
...storage/handler/impl/DataSkippableReadHandler.java 81.25% <0.00%> (-3.13%) ⬇️
.../apache/uniffle/coordinator/ClientConfManager.java 91.54% <0.00%> (-1.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f931cf0...346c6d1. Read the comment docs.

@zuston zuston requested a review from duanmeng July 16, 2022 02:36
@zuston
Copy link
Member Author

zuston commented Jul 16, 2022

@duanmeng @jerqi Updated

Changelog

  1. Rename the reservedData to extraProperties

@roryqi
Copy link
Contributor

roryqi commented Jul 16, 2022 •
edited
Loading

Is this a compatible feature? Can low version client use high version server? Can high version client use low version server?

@zuston
Copy link
Member Author

zuston commented Jul 16, 2022

I think it's an incompatible change.

@roryqi
Copy link
Contributor

roryqi commented Jul 16, 2022 •
edited
Loading

I think it's an incompatible change.

I think we can make it become a compatible feature. Because protobuffer have backwards-compatibility.

@zuston
Copy link
Member Author

zuston commented Jul 17, 2022

@jerqi
Tested. Now if the client of low version is interactive with high version coordinator, the current implementation is compatible.
The extraProperties will be a unmodified empty map.

@roryqi
Copy link
Contributor

roryqi commented Jul 17, 2022

LGTM, let @duanmeng take a look and decide whether to approve.

@zuston zuston force-pushed the delegateAccessFix2 branch from f3be39f to 346c6d1 Compare July 18, 2022 06:09
@duanmeng duanmeng changed the title Introduce the reservedData to extend more pluggable accessCheckers Introduce the extraProperties to support user-defined pluggable accessCheckers Jul 18, 2022
duanmeng
duanmeng approved these changes Jul 18, 2022
View reviewed changes
Copy link
Contributor

@duanmeng duanmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@duanmeng duanmeng merged commit bdffcaa into apache:master Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roryqi roryqi roryqi left review comments

@duanmeng duanmeng duanmeng approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zuston @roryqi @codecov-commenter @duanmeng